-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add FileSystemHandle.remove method #9
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need to make your membership of the googlers GitHub organization public to appease the IPR bot.
Removes the extra check added in https://crrev.com/c/3123735. See recent discussion on the spec here: whatwg/fs#9 (comment) Bug: 1114923, 1399660 Change-Id: I0afd561238ae44dfa055e6fff234e801ee8de616
Spec PR: whatwg/fs#9 Bug: 1114923 Change-Id: Ie52068e0053cf3a1b693e09c5623eb4b42dd6eb9
Removes the extra check added in https://crrev.com/c/3123735. See recent discussion on the spec here: whatwg/fs#9 (comment) Bug: 1114923, 1399660 Change-Id: I0afd561238ae44dfa055e6fff234e801ee8de616 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4090632 Auto-Submit: Austin Sullivan <[email protected]> Reviewed-by: Daseul Lee <[email protected]> Commit-Queue: Austin Sullivan <[email protected]> Cr-Commit-Position: refs/heads/main@{#1081696}
Removes the extra check added in https://crrev.com/c/3123735. See recent discussion on the spec here: whatwg/fs#9 (comment) Bug: 1114923, 1399660 Change-Id: I0afd561238ae44dfa055e6fff234e801ee8de616 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4090632 Auto-Submit: Austin Sullivan <[email protected]> Reviewed-by: Daseul Lee <[email protected]> Commit-Queue: Austin Sullivan <[email protected]> Cr-Commit-Position: refs/heads/main@{#1081696}
Removes the extra check added in https://crrev.com/c/3123735. See recent discussion on the spec here: whatwg/fs#9 (comment) Bug: 1114923, 1399660 Change-Id: I0afd561238ae44dfa055e6fff234e801ee8de616 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4090632 Auto-Submit: Austin Sullivan <[email protected]> Reviewed-by: Daseul Lee <[email protected]> Commit-Queue: Austin Sullivan <[email protected]> Cr-Commit-Position: refs/heads/main@{#1081696}
Spec PR: whatwg/fs#9 Bug: 1114923 Change-Id: Ie52068e0053cf3a1b693e09c5623eb4b42dd6eb9
Spec PR: whatwg/fs#9 Bug: 1114923 Change-Id: Ie52068e0053cf3a1b693e09c5623eb4b42dd6eb9 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4073126 Reviewed-by: Daseul Lee <[email protected]> Commit-Queue: Austin Sullivan <[email protected]> Auto-Submit: Austin Sullivan <[email protected]> Cr-Commit-Position: refs/heads/main@{#1081718}
Spec PR: whatwg/fs#9 Bug: 1114923 Change-Id: Ie52068e0053cf3a1b693e09c5623eb4b42dd6eb9 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4073126 Reviewed-by: Daseul Lee <[email protected]> Commit-Queue: Austin Sullivan <[email protected]> Auto-Submit: Austin Sullivan <[email protected]> Cr-Commit-Position: refs/heads/main@{#1081718}
Spec PR: whatwg/fs#9 Bug: 1114923 Change-Id: Ie52068e0053cf3a1b693e09c5623eb4b42dd6eb9 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4073126 Reviewed-by: Daseul Lee <[email protected]> Commit-Queue: Austin Sullivan <[email protected]> Auto-Submit: Austin Sullivan <[email protected]> Cr-Commit-Position: refs/heads/main@{#1081718}
I've attempted to do this in the latest patchset (thank you for finishing off whatwg/storage#155!), but I'm not sure whether we should be queueing everything or just the bits that may touch quota? (in this case, the actual removal of the entry). This could alternatively look like:
...but based on your comment here (#3 (comment)) it seems like the promise must be resolved/rejected on some task source, so maybe it should be this?
Please let me know if you have a preference :) |
I realized when putting together #95 that (I think) the parallel queue can be hidden to just the lock acquiring/releasing algorithms so the "Grab lock" step may actually not have to change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to have a good delineation between the main thread and the storage thread operations.
The method and the eventual task run in the main thread and can access JS.
What's missing here is an "in parallel" section that runs in some other thread and deals with locks and the file system.
Or am I misunderstanding something?
You're correct, and I've updated this in the latest patchset. It's still not perfect since file locking is still done in parallel on the main thread, but once #95 adds the new framework for file locking we can follow that pattern. I'm happy to do that in a follow-up |
Also, #73 (comment) is relevant here, too... we may need to specify what taking locks on a directory looks like (i.e. you can't take the lock if a containing file is locked) |
|accessResult|'s [=file system access result/error name=] and | ||
abort these steps. | ||
|
||
1. If |entry| is `null`, [=queue a storage task=] with |global| to [=/reject=] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: https://infra.spec.whatwg.org doesn't use markup for null.
1. Attempt to remove |child| from the underlying file system. | ||
If that throws an exception, [=/reject=] |result| with | ||
that exception and abort these steps. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems weird that this would happen on the main thread.
|
||
Issue(11): Better specify what possible exceptions this could throw. | ||
|
||
1. [=Enqueue the following steps=] to the [=file system queue=]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need nested file system queue steps?
Migrated from WICG/file-system-access#283 (see #2)
Addresses WICG/file-system-access#214
Adds the ability for a file or directory to delete itself. See the proposal: https://github.com/whatwg/fs/blob/main/proposals/Remove.md
Currently, it is not possible to remove a file or directory given its handle. You must obtain the handle of the parent directory and call
FileSystemDirectoryHandle.removeEntry()
.This would enable the common use case where you obtain a file handle from
showSaveFilePicker()
, but then decide you don't want to save after all, and delete the file, or clearing an entire bucket file system.(See WHATWG Working Mode: Changes for more details.)
Preview | Diff